@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-backend-spir-v

Author: Sameer Sahasrabuddhe (ssahasra)

diff --git a/llvm/lib/Target/SPIRV/Analysis/SPIRVConvergenceRegionAnalysis.cpp b/llvm/lib/Target/SPIRV/Analysis/SPIRVConvergenceRegionAnalysis.cpp
index cc6daf7ef34426..9ac0f8700a3f66 100644
--- a/llvm/lib/Target/SPIRV/Analysis/SPIRVConvergenceRegionAnalysis.cpp
+++ b/llvm/lib/Target/SPIRV/Analysis/SPIRVConvergenceRegionAnalysis.cpp
@@ -56,20 +56,8 @@ getConvergenceTokenInternal(BasicBlockType *BB) {
       "Output type must be an intrinsic instruction.");
 
   for (auto &I : *BB) {
-    if (auto *II = dyn_cast<IntrinsicInst>(&I)) {
-      switch (II->getIntrinsicID()) {
-      case Intrinsic::experimental_convergence_entry:
-      case Intrinsic::experimental_convergence_loop:
-        return II;
-      case Intrinsic::experimental_convergence_anchor: {
-        auto Bundle = II->getOperandBundle(LLVMContext::OB_convergencectrl);
-        assert(Bundle->Inputs.size() == 1 &&
-               Bundle->Inputs[0]->getType()->isTokenTy());
-        auto TII = dyn_cast<IntrinsicInst>(Bundle->Inputs[0].get());
-        assert(TII != nullptr);
-        return TII;
-      }
-      }
+    if (auto *CI = dyn_cast<ConvergenceControlInst>(&I)) {
+      return CI;
     }
 
     if (auto *CI = dyn_cast<CallInst>(&I)) {

Do you have a test case? I'd like to see how this effects the generation of the merge instructions. Thanks.

Do you have a test case? I'd like to see how this effects the generation of the merge instructions. Thanks.

The way I see it, the request should be turned around. Is there an existing test case that even reaches here? This code is clearly wrong and cannot possibly do anything. Any test for it will be rejected by the verifier.

https://llvm.org/docs/ConvergentOperations.html#llvm-experimental-convergence-anchor

It is an error to pass a convergencectrl operand bundle at a call to this intrinsic.

Is there an existing test case that even reaches here?

That is a good question. I thought there would be. If you have a valid use of Intrinsic::experimental_convergence_anchor, then it would execute that case in the old code, and would assert. That would be a test case that executes the old code, and shows the need for it to be fixed.

Your change would avoid the assert, and do something. However, I'm not convinced that what it is doing is the right thing.

Sorry for my ignorance, but I'm not sure what a reasonable and expected us of the anchor is. That is why I cannot reason about it very well. I'm assuming the existing test do not use the anchor intrinsic because it would have either failed the verifier (as you said) or asserted.

I really don't understand what assert you are talking about. The code that I deleted tries to return a token used by an anchor, which cannot exist because a legal anchor should not have an operand bundle. I don't know how to be clearer than this. That code is wrong, and it should be removed.

As a concession to existing SPIRV implementation, added an assert that catches any incorrectly placed tokens on the anchor and entry intrinsics at this point. Anything that trips this assert is a bug in the use of these intrinsics and needs to be fixed separately. I don't think anything more is possible in this patch.